Replace smallest-first eviction with LRU in FileBasedCacheManager#152
Merged
JacksonKaunismaa merged 3 commits intomainfrom Feb 16, 2026
Merged
Replace smallest-first eviction with LRU in FileBasedCacheManager#152JacksonKaunismaa merged 3 commits intomainfrom
JacksonKaunismaa merged 3 commits intomainfrom
Conversation
add_entry did not handle re-adding an existing bin (which happens when save_cache writes to disk and a later maybe_load_cache reloads the grown bin). Three bugs resulted: 1. Self-eviction: the old (smaller) size in self.sizes made the bin a prime candidate for smallest-first eviction during free_space_for, removing it from in_memory_cache while add_entry was still running. The subsequent re-addition to self.sizes left the two dicts out of sync, causing a KeyError on the next eviction cycle. 2. Double-counting: when the bin was not the smallest and survived eviction, self.sizes was overwritten with the new size but total_usage_mb had the new size added without subtracting the old, inflating the counter and triggering premature eviction. 3. Memory leak on oversized entries: when free_space_for returned False, the entry remained in in_memory_cache but was never tracked in sizes, leaking memory invisibly. Fix: remove old size tracking before the eviction check, and clean up in_memory_cache when the entry cannot fit.
The previous memory management layer had three bugs caused by add_entry not handling re-added bins (when save_cache grows a bin on disk and maybe_load_cache reloads it): 1. Self-eviction: the eviction loop could pick the entry being added (its old small size made it the smallest), removing it from in_memory_cache while add_entry continued to re-add it to sizes. Next eviction cycle: KeyError. 2. Double-counting: re-adding without subtracting the old size inflated total_usage_mb, triggering premature eviction. 3. Oversized entry leak: entries that exceeded the cache limit stayed in in_memory_cache but were invisible to the size tracker. This commit replaces the entire eviction layer: - OrderedDict for LRU ordering instead of smallest-first via min() - Remove old tracking before eviction check (prevents self-eviction) - touch() method for cache hits in maybe_load_cache - Simpler code: 3 methods (add_entry, _evict_lru, touch) replace 3 (add_entry, remove_entry, free_space_for) Verified end-to-end: old code crashes with KeyError at max_mem_usage_mb=5 on 700+ pair variants; new code completes all 12 variants.
JacksonKaunismaa
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OrderedDict), fixing aKeyErrorcrash and reducing cache thrashingtouch()method called on cache hits inmaybe_load_cacheto maintain correct LRU orderingadd_entry,_evict_lru,touch) replace 3 (add_entry,remove_entry,free_space_for)Problem
add_entrydidn't handle re-adding a bin that was already in memory (which happens whensave_cachegrows a bin on disk andmaybe_load_cachereloads it). Three bugs resulted:in_memory_cachewhileadd_entrywas still running, then re-added it tosizes— leaving the two dicts out of sync. Next eviction cycle:KeyErrorat line 150.total_usage_mb, triggering premature eviction.in_memory_cachebut were invisible to size tracking.Beyond the crash, smallest-first eviction is a poor policy for this workload: small bins (often the current working set) get evicted while large stale bins persist, causing thrashing under memory pressure.
Fix
The first commit is a minimal 6-line fix that prevents the crash within the existing design. The second commit replaces the eviction layer entirely with LRU, which also fixes the crash but additionally eliminates the thrashing behavior.
Test plan
tests/test_cache_manager.py: 4 regression tests for the crash/double-counting/leak bugs, 3 tests for LRU ordering andtouch()behaviorKeyErroratmax_mem_usage_mb=5on 700+ pair variants; LRU rewrite completes all 12 variants